Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/time functions #91

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

nikhilkalige
Copy link
Contributor

Started adding the time functions.

Prop testing is kind of hard to do, as there is no way of passing correct type info to the the prop test strategy, any thoughts on what could be done?

src/timefns.c Outdated
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't want to include the Emacs C source files

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will remove them, I added them by mistake.


defvar!(CURRENT_TIME_LIST, true);

const LO_TIME_BITS: u32 = 16;
const TIMESPEC_HZ: u64 = 1000000000;
const LOG10_TIMESPEC_HZ: u32 = 9;
Copy link
Owner

@CeleritasCelery CeleritasCelery Dec 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you run cargo clippy --workspace and cargo fmt --workspace?

@CeleritasCelery
Copy link
Owner

Prop testing is kind of hard to do, as there is no way of passing correct type info to the the prop test strategy, any thoughts on what could be done?

you can add a new ObjectType to the list for LispTime here:

fn strategy(self) -> BoxedStrategy<ArbitraryObjectType> {

And then in the strategy function, we can create a strategy that only creates valid time string (or maybe valid stings 90% of the time) and then passes them to the function. That will make the prop testing actually useful.

Another thing I have been considering is creating a proc macro like this

#[defun]
#[elprop(<spec>, <spec>)]
fn time_add<'ob>(a: LispTime, b: LispTime, cx: &'ob Context) -> Object<'ob> {

where you can specify exactly what the valid inputs should be (for example in the base64 case, the function is only meaningful on ascii and raw bytes).

@nikhilkalige
Copy link
Contributor Author

I completely agree about the custom elprop macro, that would allow us to created custom prop testing features. Let me see if I can think of something

@CeleritasCelery
Copy link
Owner

I improved elprop again to better handle panics and avoid timeouts. It still does not have special handling for time functions but it works much better now. I was able to find a handful of overflow issues with the time-add function.

@CeleritasCelery
Copy link
Owner

I added a new elprop proc macro like we discussed. I pushed an example of using it with time-add to this branch. That should help find any remaining issues in this function. I also added it to base64-encode-string.

@nikhilkalige
Copy link
Contributor Author

Nice, I will continue fixing my functions.

Also was wondering if it would be better to move the elprop tests into tests rather than converting it to json and then using a external runner. I feel that would give more room to create proptests in a easier fashion.

@CeleritasCelery
Copy link
Owner

elprop tests into tests rather than converting it to json

not entirely sure what you mean. We can't convert them into Rust tests because we need to feed them to both Emacs and Rune.

@nikhilkalige
Copy link
Contributor Author

nikhilkalige commented Jan 3, 2025

I meant why can they not be just normal rust tests, like

mod tests {
  #[test]
  fn elprop_time_add() { .. }
}

We can't convert them into Rust tests because we need to feed them to both Emacs and Rune.

I don't know much about this, so I could be wrong, but I don't understand why that would be problem. I was looking at the Neovim based tests in the Zed editor and they run tests by passing commands to both Zed and a neovim backend in a test I think.

@CeleritasCelery
Copy link
Owner

CeleritasCelery commented Jan 4, 2025

I suppose you are right about that. We could have it just call it from a normal Rust test. However the structure is a little complex. Here is a diagram of all the executables involved in running elprop.

image

On the top we have a shell script to make sure we are never running on old code. Then we have the top-level elprop binary which parses the Rust source code and extracts all the functions. It is here that we run into an issue; Emacs can't read input from stdin, or send data to stdout (except for batch error messages). But it can connect to stdin/stout of its own subprocesses. So we can't just call Emacs and Rune together to test them. Instead what we do is create a middleware binary called runner.rs. This is called as a subprocess of Emacs and can send data to both Emacs and Rune and compare the results. When it is done, we write out a JSON of the results which gets read by elprop.rs and the program terminates. If you think there is a way to simplify this. let me know!

So we could call elprop from inside a regular Rust test (using std::process::Command), but I don't think we could move all the code into Rune.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants